Skip to content

refactor(aria/tabs): remove unnecessary LabelControl usage#33148

Merged
adolgachev merged 1 commit intoangular:mainfrom
adolgachev:aria-tab-labels
Apr 25, 2026
Merged

refactor(aria/tabs): remove unnecessary LabelControl usage#33148
adolgachev merged 1 commit intoangular:mainfrom
adolgachev:aria-tab-labels

Conversation

@adolgachev
Copy link
Copy Markdown
Contributor

@adolgachev adolgachev commented Apr 24, 2026 •
edited
Loading

Remove unnecessary usage of LabelControl in Tab pattern. It doesn't help if just used for setting aria-labelledby as just a lot of redirection to accomplish the same thing.

See #33146 for subsequent work to make LabelControl easier to use, and then to actually use it for the full usage allowing users of the directives to supply custom aria-labelledby or aria-label.

@adolgachev adolgachev added target: patch This PR is targeted for the next patch release action: review The PR is still awaiting reviews from at least one requested reviewer area: aria/tabs target: major This PR is targeted for the next major release and removed area: aria/tabs target: patch This PR is targeted for the next patch release labels Apr 24, 2026
@adolgachev adolgachev marked this pull request as ready for review April 24, 2026 20:11
@pullapprove pullapprove Bot requested review from andrewseguin and tjshiu April 24, 2026 20:11
@adolgachev adolgachev requested review from ok7sai and removed request for andrewseguin April 24, 2026 20:11
ok7sai
ok7sai reviewed Apr 24, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@ok7sai ok7sai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why remove it instead of just reworking the LabelControl and properly bind to the directive.

@adolgachev
Copy link
Copy Markdown
Contributor Author

adolgachev commented Apr 25, 2026

I don't understand why remove it instead of just reworking the LabelControl and properly bind to the directive.

See the PR notes. I have a PR for that but easier to just remove the usage first as it has no benefit. Then we can discuss the approach and not have this single unnecessary usage get in the way of reworking it.

@ok7sai
Copy link
Copy Markdown
Member

ok7sai commented Apr 25, 2026 •
edited
Loading

I see the PR note but I don't see a reworked LabelControl other than changing the defaultLabelledBy type from string[] to string | undefined. What's the plan? It would be better to include both the reworked LabelControl along with the updated Tabs usage as a POC for the reworked version.

ok7sai
ok7sai approved these changes Apr 25, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@ok7sai ok7sai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you created a CL already feel free to remove it.

@adolgachev adolgachev removed the request for review from tjshiu April 25, 2026 22:43
@adolgachev adolgachev added action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release target: major This PR is targeted for the next major release and removed action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release target: patch This PR is targeted for the next patch release labels Apr 25, 2026
@adolgachev adolgachev merged commit 13c92a3 into angular:main Apr 25, 2026
32 of 34 checks passed
@adolgachev
Copy link
Copy Markdown
Contributor Author

adolgachev commented Apr 25, 2026

This PR was merged into the repository. The changes were merged into the following branches:

@adolgachev adolgachev deleted the aria-tab-labels branch April 25, 2026 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ok7sai ok7sai ok7sai approved these changes

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker area: aria/tabs target: major This PR is targeted for the next major release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@adolgachev @ok7sai